import environment variables that are present #1019

ktomk · 2018-04-22T15:55:17Z

previously docker did import environment variables if they were present
but created them if they were not when it was asked via a --env-file
cli option to import but not create them.

fix is to only import the variable into the environment if it is present.

additionally do not import variable names of zero-length (which are lines
w/ a potential variable definition w/o a variable name).

refs:

Docker sets environment variable from --env-file to zero-length-string that was meant to be taken if exported only for-linux#284

- What I did

Check for environment variable presence and only import if it is present.
Check the environment variable is not with a zero-length name

- How I did it

Swap os.Getenv w/ os.LookupEnv for emptyFn to gain presence information and handle it
String length check

- How to verify it

I have not run any automated test on these apart from the existing build. I'm new to go so I'm not yet comfortable adding tests.

- Description for the changelog

Fix environment file parsing for imports of absent variables and those w/ no name.

ktomk · 2018-04-22T15:59:15Z

I need some assistance w/ this PR: I reported the issue in the docker-linux repository and I don't know if that is the right location as I could identfiy the cli repository to create the PR later so was unsure whether or not it is necessary to re-create the issue or how to cross-link it in the description.

vdemeester

SGTM 🐯

thaJeztah · 2018-05-22T14:25:48Z

Thanks! I double checked the expected behavior as I know there were some oddities in this in the past 😅

From the documentation: set environment variables (-e, --env, --env-file)

When running the command, the Docker CLI client checks the value the variable has in your local environment and passes it to the container. If no = is provided and that variable is not exported in your local environment, the variable won’t be set in the container.

Using the -e / --env option seems to match that behavior (removed the default HOME, PATH, and HOSTNAME vars in the output):

export FROMENVIRONMENT=blabla

docker run --rm -e FROMENVIRONMENT -e NOSUCHVAR -e NOSUCHWITHEQUAL= busybox env

FOOBAR=blabla
NOSUCHWITHEQUAL=

But doing the same through an env-file indeed produces a different result:

cat <<EOF > myenv
FROMENVIRONMENT
NOSUCHVAR
NOSUCHWITHEQUAL=
EOF

docker run --rm --env-file ./myenv busybox env

FROMENVIRONMENT=
NOSUCHVAR=
NOSUCHWITHEQUAL=

Testing with this patch correctly produces

docker run --rm --env-file ./myenv busybox env

FROMENVIRONMENT=blabla
NOSUCHWITHEQUAL=

thaJeztah

Left some minor nits, but would like to see a test / integration test for this.

There were some tests in the Moby repository, but those were removed there after the CLI moved to its own repository (see moby/moby#33511)

@vdemeester do you know if new tests were added in this repository to replace those?

thaJeztah · 2018-05-22T14:40:01Z

opts/file.go

 				if emptyFn != nil {
-					value = emptyFn(line)
+					value, boolean = emptyFn(line)


boolean is a bit odd for the variable name; perhaps change to something like exists or isSet

Sure for the rename.

For the tests I need some assistance but I'm eager to add some / extend existing ones, I just need some guidance into.

thaJeztah · 2018-05-22T14:40:38Z

opts/file.go

@@ -53,17 +53,23 @@ func parseKeyValueFile(filename string, emptyFn func(string) string) ([]string,
 			if strings.ContainsAny(variable, whiteSpaces) {
 				return []string{}, ErrBadKey{fmt.Sprintf("variable '%s' has white spaces", variable)}
 			}
+			if len(variable) == 0 {
+				return []string{}, ErrBadKey{fmt.Sprintf("no variable name on line '%s'", line)}


Also testing the validation, using this env-file;

FROMENVIRONMENT # this is a comment NOSUCHVAR NOSUCHWITHEQUAL= # empty line: # whitespace-only line (spaces): # whitespace-only line (tabs): # whitespace-only line (tabs + spaces): # starts with = (invalid) =nothing EOF

$ docker run --rm --env-file ./myenv busybox env docker: poorly formatted environment: no variable name on line '=nothing'. See 'docker run --help'.

Added the test for the fix in question before the fix. signed both commits. I rbased fresh on master, I guess this goes easily into the RC but I don't know of the Docker projects internal workflow regarding versioning, so just FYI. Let me know if you'd like to get TESTING.md cleaned up or about any back ports.

silvin-lubecki

LGTM

silvin-lubecki · 2018-05-22T15:23:37Z

opts/file.go

@@ -53,17 +53,23 @@ func parseKeyValueFile(filename string, emptyFn func(string) string) ([]string,
 			if strings.ContainsAny(variable, whiteSpaces) {
 				return []string{}, ErrBadKey{fmt.Sprintf("variable '%s' has white spaces", variable)}
 			}
+			if len(variable) == 0 {
+				return []string{}, ErrBadKey{fmt.Sprintf("no variable name on line '%s'", line)}
+			}

 			if len(data) > 1 {
 				// pass the value through, no trimming
 				lines = append(lines, fmt.Sprintf("%s=%s", variable, data[1]))
 			} else {
 				var value string


nit:

if emptyFn != nil{ if value, isSet := emptyFn(line); isSet{ // if only a pass-through variable is given, clean it up. lines = append(lines, fmt.Sprintf("%s=%s", strings.TrimSpace(line), value)) } }

It's not suggested within POSIX that a well-formed variable name may contain a space character, however: "Other characters may be permitted by an implementation; applications shall tolerate the presence of such names." - so either refuse or accept but not alter (like trimming spaces). (quote from: http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap08.html)

At least needs tests against emptyFn() implementation how it deals with such thought-to-be-trim-able variable names as it is used to acquire the value. The behavior must then be same.

Yes, we had some validation in the past, but there's no strict rule what's "allowed" as variable name, so we decided to remove validation and leave it to the container's process to decide what's valid and what not.

Having said the above; trimming trailing whitespace in variable names could be an exception (i.e., I don't think it's possible to define a var with leading/trailing whitespace, as the whitespace would be stripped by the shell), but I'm not sure we should; for example, the following would produce an error in your shell as well;

$ export foo = bar bash: export: `=': not a valid identifier

Leading whitespace in the value leads to "no value being set";

$ export bar= bar $ env | grep bar bar=

This looks to be something where we differ, because the shell requires quotes for that:

$ export baz=" bla" $ env | grep baz baz= bla

My nit was only a small refactor and should be equivalent with your version.

Or maybe were you answering to @thaJeztah previous comment 😅

thaJeztah · 2018-05-24T20:59:23Z

@ktomk for the tests; I think we need an integration test for this; there are some existing tests in https://github.com/docker/cli/blob/master/e2e/image/build_test.go, which could be a good starting point to look at.

Let me know if that's enough information, or if you need more 👍

ktomk · 2018-05-24T22:01:20Z

@thaJeztah: Thanks for the follow up on tests, I'll take a look. From a very first glance, how to invoke (run) a test file then?

/e: after pressing the submit button, reminded me I can check the cicleci test step probably for that info.

thaJeztah · 2018-05-24T22:07:09Z

Just recalled we have some docs on that in https://github.com/docker/cli/blob/master/TESTING.md

ktomk · 2018-07-01T21:25:01Z

@thaJeztah; TESTING.md is lying about package github.com/stretchr/testify/assert, it should be removed from that document, tests aren't even executed w/ it as the package is not even loaded. I don't know if opening a ticket is on your side as you're the reviewer requesting tests based on that wrong information, regardless of what, just from my end, please see it reported by my comment.

test to show current behavior is wrong at parsing an environment file defining an undefined variable - it must not be defined! NOTE: this test assume the $HOME variable is always set (see POSIX, this normally is the case, e.g. the test suite remains stable). Signed-off-by: Tom Klingenberg <tklingenberg@lastflood.net>

previously docker did import environment variables if they were present but created them if they were not when it was asked via a --env-file cli option to import but not create them. fix is to only import the variable into the environment if it is present. additionally do not import variable names of zero-length (which are lines w/ a potential variable definition w/o a variable name). refs: - docker/for-linux#284 Signed-off-by: Tom Klingenberg <tklingenberg@lastflood.net>

parsing an environment file should give an error in case a zero-length variable name (definition w/o a variable name) is encountered. previously these lines went through unnoticed not informing the user about a potential configuration error. Signed-off-by: Tom Klingenberg <tklingenberg@lastflood.net>

ktomk · 2018-07-12T04:00:25Z

@thaJeztah Please review the tests I added and let me know about any ifs.

thaJeztah

LGTM, thanks!

vdemeester

still LGTM 🐯

GordonTheTurtle added the status/0-triage label Apr 22, 2018

ktomk force-pushed the fix-env-file branch from db740f6 to 1252a11 Compare April 22, 2018 17:52

vdemeester approved these changes Apr 23, 2018

View reviewed changes

vdemeester requested review from thaJeztah and silvin-lubecki April 23, 2018 08:30

vdemeester added status/2-code-review and removed status/0-triage labels Apr 23, 2018

thaJeztah requested changes May 22, 2018

View reviewed changes

silvin-lubecki approved these changes May 22, 2018

View reviewed changes

ktomk force-pushed the fix-env-file branch from 1252a11 to 428cf2d Compare May 23, 2018 17:47

ktomk force-pushed the fix-env-file branch from 428cf2d to 5144ed3 Compare July 1, 2018 21:18

GordonTheTurtle added the dco/no label Jul 1, 2018

ktomk force-pushed the fix-env-file branch from 5144ed3 to 9ed2e9d Compare July 1, 2018 21:34

GordonTheTurtle added the dco/no label Jul 1, 2018

ktomk force-pushed the fix-env-file branch from 9ed2e9d to ea711c5 Compare July 1, 2018 21:41

GordonTheTurtle added the dco/no label Jul 1, 2018

ktomk force-pushed the fix-env-file branch from 290a81a to be56091 Compare July 1, 2018 21:56

GordonTheTurtle removed the dco/no label Jul 1, 2018

ktomk added 3 commits July 2, 2018 07:33

ktomk force-pushed the fix-env-file branch from be56091 to b91fd12 Compare July 2, 2018 05:52

docker deleted a comment from GordonTheTurtle Jul 2, 2018

thaJeztah approved these changes Jul 16, 2018

View reviewed changes

vdemeester approved these changes Jul 16, 2018

View reviewed changes

vdemeester merged commit 9e71207 into docker:master Jul 16, 2018

GordonTheTurtle added this to the 18.07.0 milestone Jul 16, 2018

ndeloof mentioned this pull request Jan 16, 2020

Include unset variables when parsing env_file #2259

Closed

GitSoftwareNow mentioned this pull request May 25, 2020

what's meaning of Exit code 139? #2545

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

import environment variables that are present #1019

import environment variables that are present #1019

ktomk commented Apr 22, 2018 •

edited by thaJeztah

Loading

ktomk commented Apr 22, 2018

vdemeester left a comment

thaJeztah commented May 22, 2018

thaJeztah left a comment

thaJeztah May 22, 2018

ktomk May 22, 2018

thaJeztah May 22, 2018

ktomk Jul 1, 2018 •

edited

Loading

silvin-lubecki left a comment

silvin-lubecki May 22, 2018

ktomk May 22, 2018

ktomk May 22, 2018

thaJeztah May 22, 2018

silvin-lubecki May 23, 2018

silvin-lubecki May 23, 2018

thaJeztah commented May 24, 2018

ktomk commented May 24, 2018 •

edited

Loading

thaJeztah commented May 24, 2018

ktomk commented Jul 1, 2018 •

edited

Loading

ktomk commented Jul 12, 2018

thaJeztah left a comment

vdemeester left a comment

import environment variables that are present #1019

import environment variables that are present #1019

Conversation

ktomk commented Apr 22, 2018 • edited by thaJeztah Loading

ktomk commented Apr 22, 2018

vdemeester left a comment

Choose a reason for hiding this comment

thaJeztah commented May 22, 2018

thaJeztah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ktomk Jul 1, 2018 • edited Loading

Choose a reason for hiding this comment

silvin-lubecki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah commented May 24, 2018

ktomk commented May 24, 2018 • edited Loading

thaJeztah commented May 24, 2018

ktomk commented Jul 1, 2018 • edited Loading

ktomk commented Jul 12, 2018

thaJeztah left a comment

Choose a reason for hiding this comment

vdemeester left a comment

Choose a reason for hiding this comment

ktomk commented Apr 22, 2018 •

edited by thaJeztah

Loading

ktomk Jul 1, 2018 •

edited

Loading

ktomk commented May 24, 2018 •

edited

Loading

ktomk commented Jul 1, 2018 •

edited

Loading